fix(knife): GPG-verify Node.js SHASUMS256.txt.asc before committing checksums - #2147
Conversation
…checksums Signed-off-by: rishuranjan <rishuranjan6@gmail.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
I've submitted this pull request and would appreciate a review when you have some bandwidth. Please let me know if any changes or additional context are needed. Thank you for your time and consideration. |
|
Hi @loosebazooka @nlopezgi @bobcallaway Gentle ping on #2147. The PR adds GPG verification of SHASUMS256.txt.asc before committing Node.js checksums, closing a supply chain attack vector in the nightly update-node-archives workflow. Happy to address any feedback. Thanks! |
|
I am not a node person but did a quick check. I feel the |
|
Hi @lathama Thanks for the review! You're right - fetching from a keyserver at runtime breaks in restricted network environments, and using execSync for shell commands is fragile. Better approach: bundle the Node.js release team's public keys directly in the repo as a committed file (sourced from https://github.com/nodejs/node#release-keys) and replace the gpg shell call with the openpgp npm package for pure Node.js verification - no system dependencies, no network calls at build time. These are public keys, not secrets - they're already published openly by the Node.js team and are safe to commit. Think of them like a list of trusted signatures you keep on file to check documents against. The only maintenance consideration is that Node.js occasionally adds new release signers. When that happens the keys file needs a one-line update via PR, which is better than fetching keys silently at runtime - changes go through review and are tracked in git history. Does this direction sound good to you, or do you have a different approach in mind? |
|
@rishuranjanofficial I don't have any better ideas, just calling out what I saw. Not sure if there is any solution to this at this time. |
|
Please don't ping random users like you've done here. The team is aware of this PR and have added it to our backlog. There is no timeline when someone will be available to review this. |
|
Apologies - I didn't mean to bother anyone, and I'll stop the pings. Thanks for the update; glad it's on the backlog. Happy to address any feedback whenever the team has bandwidth, no rush on my end. |
|
alright thanks for your patience with this:
|
|
Done - implemented as you outlined: Committed the active Node.js release keys as a single knife.d/nodejs_keys.asc (exported from nodejs/release-keys/gpg-only-active-keys via gpg). One expected, by-design behavior I'll call out so it's not a surprise (not a concern): verification is fail-closed against the committed keys. If Node.js introduces a brand-new release signer, a release signed only by that new key won't verify until the daily cron refreshes nodejs_keys.asc (or the script is run manually) - a self-healing ~1-day window. It only touches the internal update-node-archives maintenance job, not contributors, PRs, or builds, and it's strictly more secure than the previous static key list. PTAL. |
I think this is fine and should allow for manual key update by maintainers. I expect this occur somewhat infrequently. |
|
Thanks! That's everything implemented on my end: active Node.js release keys committed as knife.d/nodejs_keys.asc I believe this is ready for review - happy to make any further changes if needed. |
loosebazooka
left a comment
There was a problem hiding this comment.
a few minor things, but this seems almost done
| const base = `https://nodejs.org/dist/v${nodeVersion}`; | ||
| const [shasumsText, shasumsAsc] = await Promise.all([ | ||
| fetchText(`${base}/SHASUMS256.txt`), | ||
| fetchText(`${base}/SHASUMS256.txt.asc`), |
There was a problem hiding this comment.
looks like the .txt.asc file is inline signature, so either we use
gpg --verify SHASUM256.txt.sig SHASUM256.txtto validate the.txtusing the.sig.ascfile
orgpg --decrypt SHASUM256.txt.ascto unwrap the.txt.ascand use it
| persist-credentials: false | ||
|
|
||
| - name: Update node release keys | ||
| run: ./knife.d/update_node_keys.sh |
There was a problem hiding this comment.
I know our knife script structure is a bit silly. But update_node_keys.sh should be exposed directly in the knife script to be called like ./knife update-node-keys
There was a problem hiding this comment.
Nice catch - switched to gpg --output - --decrypt SHASUMS256.txt.asc, which verifies the inline signature and returns the authenticated checksums, and dropped the separate plaintext SHASUMS256.txt fetch so nothing unsigned is ever parsed. Validated against a real release (v22.11.0): good signature from a Node.js release signer + correct checksums, and confirmed it fails closed (non-zero exit) when the signer isn't in the committed keys. Pushed.
Problem
knife.d/update_node_archives.jscomputes SHA-256 checksums by downloadingeach Node.js tarball directly and hashing it locally. The resulting hashes are
committed to
private/extensions/node.bzland later used by Bazel to verifydownloads.
If the nodejs.org CDN, a reverse proxy, or DNS resolution is compromised at the
time the nightly
update-node-archivesworkflow runs, an attacker can serve amalicious tarball. The script will compute and commit the correct SHA-256 of the
malicious file — Bazel will then accept it on every subsequent build, silently
backdooring all
gcr.io/distroless/nodejs*images.Solution
Node.js publishes a GPG-signed
SHASUMS256.txt.ascalongside every release,signed by the Node.js release team's keys (documented at
https://github.com/nodejs/node#release-keys). Verifying this signature before
trusting any checksum establishes a chain of custody back to the Node.js release
team and eliminates the attack vector above.
This PR:
keyring at startup (with fallback to a secondary keyserver).
SHASUMS256.txtandSHASUMS256.txt.ascfor each Node.js version.gpg --verify— the script hard-fails if the signature does notvalidate, preventing any bad hashes from being committed.
SHASUMS256.txtto extract per-architecture SHA-256values, replacing the previous approach of hashing downloaded tarballs.
finallyblock.The nightly workflow already runs on
ubuntu-latest, which hasgpgpre-installed. No runner or workflow changes are required.
Security impact
Testing
Verified locally that:
by the previous tarball-hash approach.
creation.